KubeNodeKubeletV1 added - #30
Conversation
relrod
left a comment
relrod
left a comment
There was a problem hiding this comment.
Probably the most important thing this is missing is RBAC. The scrape won't actually work with out it, we need to add nodes/healthz in charts/cmk-rustik/templates/metrics-fetcher/clusterrole.yaml
| match self { | ||
| Self::KubeletStatsSummary(bytes) => bytes.clone(), | ||
| Self::KubeletHealth { health, .. } => { | ||
| Bytes::from(serde_json::to_vec(health).expect("KubeletHealth always serializes")) |
There was a problem hiding this comment.
Let's avoid .expect() here (if there's an issue serializing, we'll panic otherwise).
One option would be to make Payload::KubeletHealth look identical in structure to Payload::CheckmkLinuxAgent. Then do the serialization to JSON in your scrape(). Then this ends up looking just like the it does for the Self::CheckmkLinuxAgent case below.
| name: "NODE_NAME".to_string(), | ||
| source: e, | ||
| })?; | ||
| let token = std::fs::read_to_string("/var/run/secrets/kubernetes.io/serviceaccount/token")?; |
There was a problem hiding this comment.
Prefer tokio::fs::read_to_string().await? here
| let token = std::fs::read_to_string("/var/run/secrets/kubernetes.io/serviceaccount/token")?; | ||
|
|
||
| debug!("fetching Kubelet /healthz"); | ||
| let health = match self |
There was a problem hiding this comment.
I'd simplify this a bit by storing the intermediate result (like kubelet_stats_summary does) and doing the match below it.
i.e.
let response = self.scrape_client.....send().await;
match response {...}I'd also add a some logging (maybe pull the url into a variable and pass that in the log too):
let url = format!("https://{node_ip}:10250/healthz");
let response = ...;
let status = response.status();
if status == StatusCode::OK {
debug!(status = %response.status(), url, "kubelet healthz scrape complete");
} else {
warn!(status = %response.status(), url, "kubelet healthz scrape returned non-OK status");
}
There was a problem hiding this comment.
I would have maybe suggested having this be two separate PRs: One for ingest (get the data in from metrics-fetcher, chart changes, etc) and one for emission (everything under section/, piggyback/), just so it's easier to reason about.
But it's fine if you don't want to now, since it is already pretty close to done.
There was a problem hiding this comment.
Rather than store a direct HashMap in Snapshot, newtyping it lets us avoid exposing a mutable structure from the snapshot. And as we are going to have more of these kinds of data sources (#30), it is better to establish a pattern now.
2 participants
CMK-36238